-
-
Notifications
You must be signed in to change notification settings - Fork 462
feat(replay): Capture network request/response details when using SentryOkHttpListener #4919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Capture network request/response details when using SentryOkHttpListener ([#4919](https://github.com/getsentry/sentry-java/pull/4919))If none of the above apply, you can opt out of this check by adding |
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 604a261 | 380.65 ms | 451.27 ms | 70.62 ms |
| ee747ae | 382.73 ms | 435.41 ms | 52.68 ms |
| b77456b | 393.26 ms | 441.10 ms | 47.84 ms |
| 9fbb112 | 401.87 ms | 515.87 ms | 114.00 ms |
| 806307f | 357.85 ms | 424.64 ms | 66.79 ms |
| 27d7cf8 | 309.43 ms | 364.27 ms | 54.85 ms |
| 7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
| d217708 | 375.27 ms | 415.68 ms | 40.41 ms |
| 96449e8 | 361.30 ms | 423.39 ms | 62.09 ms |
| 1df7eb6 | 397.04 ms | 429.64 ms | 32.60 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 604a261 | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| b77456b | 1.58 MiB | 2.12 MiB | 548.11 KiB |
| 9fbb112 | 1.58 MiB | 2.11 MiB | 539.18 KiB |
| 806307f | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| 7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| 96449e8 | 1.58 MiB | 2.11 MiB | 539.35 KiB |
| 1df7eb6 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
Previous results on branch: 43jay/MOBILE-935
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fedc731 | 325.81 ms | 383.02 ms | 57.21 ms |
| da92356 | 319.12 ms | 388.73 ms | 69.61 ms |
| 154fb27 | 336.98 ms | 387.10 ms | 50.12 ms |
| 55850ea | 370.79 ms | 431.33 ms | 60.55 ms |
| 3162f1d | 308.61 ms | 367.50 ms | 58.89 ms |
| 790a163 | 339.64 ms | 436.57 ms | 96.94 ms |
| 1c4ba0c | 302.22 ms | 367.21 ms | 64.98 ms |
| 6687052 | 321.98 ms | 383.35 ms | 61.38 ms |
| 1a07f53 | 341.15 ms | 378.86 ms | 37.71 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fedc731 | 1.58 MiB | 2.13 MiB | 557.33 KiB |
| da92356 | 1.58 MiB | 2.13 MiB | 556.24 KiB |
| 154fb27 | 1.58 MiB | 2.13 MiB | 557.32 KiB |
| 55850ea | 1.58 MiB | 2.13 MiB | 557.31 KiB |
| 3162f1d | 1.58 MiB | 2.13 MiB | 557.32 KiB |
| 790a163 | 1.58 MiB | 2.12 MiB | 553.01 KiB |
| 1c4ba0c | 1.58 MiB | 2.13 MiB | 556.34 KiB |
| 6687052 | 1.58 MiB | 2.13 MiB | 556.25 KiB |
| 1a07f53 | 1.58 MiB | 2.12 MiB | 553.02 KiB |
cf95f21 to
2e9b796
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Duplicate HTTP headers lost in conversion
The toMap() function loses duplicate HTTP headers with the same name. When iterating through OkHttp headers and inserting into a Map<String, String>, duplicate header names get overwritten, keeping only the last value. This causes data loss for headers like Set-Cookie, Accept, or Vary that commonly appear multiple times in HTTP messages. The captured network details will be incomplete when responses or requests contain duplicate headers.
sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt#L265-L272
sentry-java/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt
Lines 265 to 272 in 22bb832
| /** Extracts headers from OkHttp Headers object into a map */ | |
| private fun okhttp3.Headers.toMap(): Map<String, String> { | |
| val headers = linkedMapOf<String, String>() | |
| for (i in 0 until size) { | |
| headers[name(i)] = value(i) | |
| } | |
| return headers | |
| } |
sentry/src/main/java/io/sentry/util/network/NetworkDetailCaptureUtils.java
Show resolved
Hide resolved
Run tests ./gradlew :sentry:test --tests="*NetworkDetailCaptureUtilsTest*"
Reuse existing logic that retrieves optional SentryOkHttpEvent for the okhttp3.Call, and optionally provide NetworkRequestData for adding to Breadcrumb Hint in SentryOkHttpEvent#finish
./gradlew :sentry-okhttp:test --tests="*SentryOkHttpEventTest*setNetworkDetails*"
e9a9f5f to
e6d7289
Compare
review comment - https://github.com/getsentry/sentry-java/pull/4919/files#r2550496731 seems possible, e.g. if a client passes null in the array to SentryReplayOptions#set[Request|Response]Headers
review comment - #4919 (review) ./gradlew :sentry-okhttp:test --tests="*SentryOkHttpInterceptorTest.toMap handles duplicate headers correctly*"
|
| headers[name] = "$existingValue, $value" | ||
| } else { | ||
| headers[name] = value | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Set-Cookie headers incorrectly concatenated with comma separator
The toMap() function concatenates duplicate HTTP headers with comma separators, but Set-Cookie is an exception that cannot be concatenated this way per RFC 6265. Cookie values can contain commas in attributes like expiration dates, causing the concatenated value to become unparseable. This corrupts Set-Cookie headers when multiple cookies are set, breaking network detail capture for session replay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ator Issue is that commas are valid separators in certain headers (Cookie, Set-Cookie,...). Switch to semi-colon separated instead -> this only governs the formatted list that appears in the sentry dashboard so is relatively minor. review comment - #4919 (comment) ./gradlew :sentry-okhttp:test --tests="*SentryOkHttpInterceptorTest.toMap handles duplicate headers correctly*"
📜 Description
Make network details capture (http request/response bodies and headers - docs) available to clients using the sentry gradle plugin by adding support to
SentryOkHttpEventListener.Updates CHANGELOG.
💡 Motivation and Context
Part of [Mobile Replay] Capture Network request/response bodies.
Network Details extraction depends on okhttp3.Interceptor (
SentryOkHttpInterceptor) to intercept okhttp requests and extract the relevant data to the Breadcrumb Hint (see #4796).However, the Sentry Interceptor is only responsible for sending Breadcrumb data if for some reason the
SentryOkHttpEventListeneris not used (code ref).Given registering
SentryOkHttpEventListeneris the default gradle plugin behavior, typical SDK configuration will see Breadcrumb data sent viaSentryOkHttpEventListener. So this PR modifiesSentryOkHttpEventto receive network details data from the interceptor, to be included when sending the Breadcrumb from that path.💚 How did you test it?
Unit
% ./gradlew :sentry-okhttp:test --tests="*SentryOkHttpEventTest*network*details*"Manual
Build sentry-samples:
% ./gradlew :sentry-okhttp:clean :sentry-android-replay:clean :sentry-samples:sentry-samples-android:clean :sentry-samples:sentry-samples-android:installDebug --no-build-cache --rerun-tasks1. OkHttpClient w/ Sentry Interceptor+EventListener will upload network details
Modify sentry-samples to create http requests with event listener & interceptor:

Verify session replay created as expected
https://sentry-sdks.sentry.io/explore/replays/09681100b32640b99f53fc68a016f00d/...
2. OKHttpClient w/ only Sentry Interceptor will upload network details
Comment out addEventListener line and rebuild/install:

Verify session replay created as expected
https://sentry-sdks.sentry.io/explore/replays/29ce82bbd31c482e9fb0a60122412d3d/...
3. OKHttpClient w/ NO Sentry Interceptor will NOT upload network details
Comment out

addInterceptorline and rebuild/install:Verify session replay created with no data as expected
https://sentry-sdks.sentry.io/explore/replays/1232a6f76a6e428281779e8021943429/...
📝 Checklist
sendDefaultPIIis enabled. N/A🔮 Next steps